Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃帀 Source Looker: add run-look endpoint #3911

Merged
merged 12 commits into from
Jun 25, 2021
Merged

馃帀 Source Looker: add run-look endpoint #3911

merged 12 commits into from
Jun 25, 2021

Conversation

mjirv
Copy link
Contributor

@mjirv mjirv commented Jun 5, 2021 •
edited
Loading

What

This PR adds a Run Look output stream to the Looker connector. Addresses #3649.

It works but isn't totally ready to go yet. I want to work on the following:

  1. Add better error handling (e.g. the web app seems to hang on the connector schema page if a non-existent Look ID is entered; it should show an error, or better yet validate its existence at setup).
  2. Right now all columns are treated as strings for normalization because I can't see a way to get their types from Looker.
  3. Clean up my code a little
  4. Maybe add tests (there aren't any for Looker right now)

How

Implements a stream__run_looks method in client.py along with methods to generate a json_schema from the provided look(s). Also adds a new configuration field for Look IDs to run.

Recommended reading order

  1. spec.json
  2. client.py

Pre-merge Checklist

Expand the checklist which is relevant for this PR.

Connector checklist

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • Secrets are annotated with airbyte_secret in output spec
  • Unit & integration tests added as appropriate (and are passing)
    • Community members: please provide proof of this succeeding locally e.g: screenshot or copy-paste acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • /test connector=connectors/<name> command as documented here is passing.
    • Community members can skip this, Airbyters will run this for you.
  • Code reviews completed
  • Credentials added to Github CI if needed and not already present. instructions for injecting secrets into CI.
  • Documentation updated
    • README
    • CHANGELOG.md
    • Reference docs in the docs/integrations/ directory.
  • Build is successful
  • Connector version bumped like described here
  • New Connector version released on Dockerhub by running the /publish command described here
  • No major blockers
  • PR merged into master branch
  • Follow up tickets have been created
  • Associated tickets have been closed & stakeholders notified

@mjirv mjirv marked this pull request as draft June 5, 2021 19:24
@mjirv mjirv mentioned this pull request Jun 5, 2021
Closed
@sherifnada
Copy link
Contributor

@mjirv LMK if there is anything we can help with!

@mjirv
Copy link
Contributor Author

mjirv commented Jun 12, 2021

Hi @sherifnada, I think it's ready for review now.

Any comments you all have would be appreciated, but the one thing I'm curious about is if you have any recommendations about what kind of tests to add since there aren't any on the connector yet.

@mjirv mjirv marked this pull request as ready for review June 12, 2021 20:50
@davinchia
Copy link
Contributor

@mjirv Sherif is on PTO now. I will get back to you by tomorrow

davinchia
davinchia reviewed Jun 15, 2021
View reviewed changes
Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjirv This logic seems sound to me.

Regarding tests, we do have integration tests they aren't easily modified by contributors. I'll create a follow up ticket and we'll take care of it. I'd like to have test on the the dynamic conversion logic - the _get_run_look_json_schema function and logic within - before we merge this in since that seems to be the main complexity here. Feel free to use unit_test.py.

Can you also take a look at the connector checklist? The documentation section should be the most relevant.

Can you also run the integration test locally and paste a screenshot of the successful output? Basic sanity check this is working. You'll have to place the credentials in secrets/config.json.

Thanks!

airbyte-integrations/connectors/source-looker/sample_files/full_configured_catalog.json Show resolved Hide resolved
airbyte-integrations/connectors/source-looker/source_looker/client.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-looker/source_looker/client.py Show resolved Hide resolved
airbyte-integrations/connectors/source-looker/source_looker/client.py Outdated Show resolved Hide resolved
tuliren
tuliren reviewed Jun 15, 2021
View reviewed changes
Copy link
Contributor

@tuliren tuliren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also update the documentation in source/looker.md to include this new stream?

airbyte-integrations/connectors/source-looker/sample_files/full_configured_catalog.json Show resolved Hide resolved
@github-actions github-actions bot added the area/connectors Connector related issues label Jun 19, 2021
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Jun 19, 2021
@mjirv
Copy link
Contributor Author

mjirv commented Jun 19, 2021

Thanks @davinchia and @tuliren. I've made updates for all your comments aside from adding unit tests (& still have to go through the connector checklist in more detail).

@tuliren
Copy link
Contributor

tuliren commented Jun 25, 2021

still have to go through the connector checklist in more detail

@mjirv, I guess you are probably busy with other things. It's totally fine. I will merge this PR as is in the next hour, and take care of the remaining items on the checklist in a follow up PR. Feel free to open another PR if there is anything else you want to add.

@tuliren
Copy link
Contributor

tuliren commented Jun 25, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-looker

Error: No ref found for: mjirv/looker_run_query_look

@tuliren tuliren changed the title Source Looker: Add Run Look endpoint 馃帀 Source Looker: add run-look endpoint Jun 25, 2021
@tuliren tuliren merged commit 842f10b into airbytehq:master Jun 25, 2021
@mjirv
Copy link
Contributor Author

mjirv commented Jun 25, 2021

Thank you @tuliren! Yeah, I got busy here the last couple weeks and didn't have a chance to get back to it yet. I appreciate you moving it forward!

@tuliren
Copy link
Contributor

tuliren commented Jun 25, 2021

Sure, no problem. Thank you for putting together this PR.

This was referenced Jun 25, 2021
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davinchia davinchia davinchia left review comments

@tuliren tuliren tuliren approved these changes

@ChristopheDuong ChristopheDuong Awaiting requested review from ChristopheDuong

Assignees

@tuliren tuliren

Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mjirv @sherifnada @davinchia @tuliren